Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable customizing Zstd decoding parameters. #285

Merged
merged 1 commit into from
Jul 21, 2024

Conversation

quantatic
Copy link
Contributor

@quantatic quantatic commented Jul 14, 2024

closes #284

This PR updates the minimum zstd version to 0.13.1, in order to make DParameter Clone+Copy.

@@ -16,6 +16,16 @@ impl ZstdDecoder {
}
}

pub(crate) fn new_with_params(params: &[crate::zstd::DParameter]) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should propagate the error from zstd?

Copy link
Contributor Author

@quantatic quantatic Jul 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not necessarily opposed to this, though in the interest of consistency, it appears that this library chooses to panic on construction errors for other encoders/decoders:

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's unfortunate, maybe we should add a try_ variant for these APIs?

cc @robjtede what's your thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think typically panics are acceptable when it comes to "programmer error" and I really, really don't expect decoding parameters to be user-input, or anything other than hardcoded. Therefore it's fine to me that this can panic. It's a fairly unrecoverable error for both these unwraps.

@quantatic quantatic force-pushed the main branch 2 times, most recently from dc585ee to fafcde9 Compare July 14, 2024 02:24
@quantatic
Copy link
Contributor Author

This change is technically semver-compatible with only a minor version bump on next release, though it's unclear to me how this would be compatible with a crate that locks zstd = "=0.13.0. Is such a change still considered semver-compatible?

@NobodyXu
Copy link
Collaborator

This change is technically semver-compatible with only a minor version bump on next release, though it's unclear to me how this would be compatible with a crate that locks zstd = "=0.13.0. Is such a change still considered semver-compatible?

Given that we don't expose any implementation detail, I think it can be said as semver-compatible API-wise

@robjtede robjtede added the A-semver-minor non-breaking change label Jul 21, 2024
@robjtede
Copy link
Member

robjtede commented Jul 21, 2024

Is such a change still considered semver-compatible

yes

nice change, thanks for the work :)

@NobodyXu NobodyXu added this pull request to the merge queue Jul 21, 2024
Merged via the queue into Nullus157:main with commit 9967f2e Jul 21, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-semver-minor non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support new_with_params for ZstdDecoder
3 participants